Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing ExecutionArgs export #1027

Merged
merged 3 commits into from
Sep 8, 2017
Merged

Conversation

IvanGoncharov
Copy link
Member

In #988 I forget to export ExecutionArgs in all necessary places 🤦‍♂️
This one-line PR fix that.
@wincent Can you please review it?

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Sep 7, 2017

@wincent I was wondering why Flow silently ignored missing export. So I discovered that a few files missing @flow directive.
And lexer.js had /* @flow / since the first commit, not sure how Flow is parsing such comments.
I also unify @flow directives to always be in a first line, so you can use simple shell script to find all files without it.
After enabling flow checks on more files two new issue appeared which I fixed in the separate commit.

@wincent
Copy link
Contributor

wincent commented Sep 8, 2017

Thanks @IvanGoncharov.

FWIW I don't really like the hoisting up of the @flow annotation to the top; it smells of special-casing to me and @flow shouldn't be more special than any other annotation that we might use (and we do use quite a few at FB). Having said that, consistency is good, and there is a precedent for this style, so... ¯\(ツ)

@wincent wincent merged commit eb01a23 into graphql:master Sep 8, 2017
@IvanGoncharov IvanGoncharov deleted the excuteArgs branch December 14, 2017 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants